Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: 'open in new tab' option issue & multiple link control UX issues #50602

Closed
wants to merge 7 commits into from
Closed

fix: 'open in new tab' option issue & multiple link control UX issues #50602

wants to merge 7 commits into from

Conversation

hz-tyfoon
Copy link
Contributor

@hz-tyfoon hz-tyfoon commented May 12, 2023

[This is the updated version of the PR: #50401 that maintains the current state of the link control & add the fixs on it]
Fix issues: #45741 & #43144
Also addresses some UI/UX issues mentioned here: this comment, also this comment with a video demonstration & this comment

*** What I've Done 👇👇 ***

Update: 4 & 5 in the list below has been removed/reverted from this PR, check #50602 (comment)

** 1) took a new state 'newValue'/'setNewValue' and took 'value' it's initial default. and replaced the usage of 'value' with this 'newValue' state everywhere in the index.js to make this state the source of truth. we'll need this state for couple of reasons and one being 'updating' the value using 'setNewValue' & another is replace 'onChange' call with 'setNewValue' or use both of them according to our need (more on this in the next steps to make it clear why this new state is taken, see the next steps for more clarity)

** 2) instead of passing the 'onChange' as prop to 'LinkControlSettingsDrawer', passed 'setNewValue' as prop. Because, the 'onChange' call makes the link popup modal to close without saving the other data properly. In 'settings-drawer.js' the receieved 'setNewValue' passed to the 'Settings' component as 'onChange' prop, this way toggling the 'open in new tab' won't close the modal & also will save the data in 'newValue' state properly.

** 3) in 'handleSubmit' of 'index.js' removed the if check as looks like it's unnecessary. Because if user decide to submit without making any change to 'url' or 'title' there's no harm in still submitting it, also there's other change can happen like 'open in new tab settings' and user may just wanna submit that. Another thing done it 'handleSubmit' & that is called both 'setNewValue' and 'onChnage' to make sure the 'local state' (newValue) stays sync with the submitted data (otherwise there may be unwanted behaviour where 'link-control component' is used. i.e. 'button block')

** 4) imported the 'Settings' component inside 'index.js' and used it below the 'LinkPreview' component. used the neccessary css in 'style.scss' to properly style it. In this 'Settings' component's 'onChange' prop, passed a function & made sure the value gets called with both 'setNewValue' and 'onChnge'. This is done for the same reason of keeping the both state sync. This settings component showen there to keep the 'open in new tab option' visible when previewing the link (after initially any richtext is linked by selecting the text & then pasting the link ). This was done to keep it consistent with 'WordPress 6.2 core UI' and looks like many people find it better UX to keep the 'option' there. See these comments for more info: this comment, also this comment with a video demonstration & this comment

** 5) finally 2 extra works done related to keeping the settings panel visible when opening the modal as seems like it's a better UX to keep this option visible rather than keeping them hidden and then requiring 1 extra click to make them open. To achieve this set the initial default value to 'true' for 'settingsOpen' useState & removed the 'setSettingsOpen(false)' from 'stopEditing' function.

*** Thanks 😊😊 ***

*** What I've Done 👇👇 ***

** 1) took a new state 'newValue'/'setNewValue' and took 'value' it's initial default. and replaced the usage of 'value' with this 'newValue' state everywhere in the index.js to make this state the source of truth. we'll need this state for couple of reasons and one being 'updating' the value using 'setNewValue' & another is replace 'onChange' call with 'setNewValue' or use both of them according to our need (more on this in the next steps to make it clear why this new state is taken, see the next steps for more clarity)

** 2) instead of passing the 'onChange' as prop to 'LinkControlSettingsDrawer', passed 'setNewValue' as prop. Because, the 'onChange' call makes the link popup modal to close without saving the other data properly. In 'settings-drawer.js' the receieved 'setNewValue' passed to the 'Settings' component as 'onChange' prop, this way toggling the 'open in new tab' won't close the modal & also will save the data in 'newValue' state properly.

** 3) in 'handleSubmit' of 'index.js' removed the if check as looks like it's unnecessary. Because if user decide to submit without making any change to 'url' or 'title' there's no harm in still submitting it, also there's other change can happen like 'open in new tab settings' and user may just wanna submit that. Another thing done it 'handleSubmit' & that is called both 'setNewValue' and 'onChnage' to make sure the 'local state' (newValue) stays sync with the submitted data (otherwise there may be unwanted behaviour where 'link-control component' is used. i.e. 'button block')

** 4) imported the 'Settings' component inside 'index.js' and used it below the 'LinkPreview' component. used the neccessary css in 'style.scss' to properly style it. In this 'Settings' component's 'onChange' prop, passed a function & made sure the value gets called with both 'setNewValue' and 'onChnge'. This is done for the same reason of keeping the both state sync. This settings component showen there to keep the 'open in new tab option' visible when previewing the link (after initially any richtext is linked by selecting the text & then pasting the link ). This was done to keep it consistent with 'WordPress 6.2 core UI' and looks like many people find it better UX to keep the 'option' there. See these comments for more info: 1) https://github.com/WordPress/gutenberg/issues/49091\#issuecomment-1477057235 2) https://github.com/WordPress/gutenberg/issues/49091\#issuecomment-1477084979 (see the video in this comment for more clarity) 3) https://github.com/WordPress/gutenberg/issues/49091\#issuecomment-1477097292

** 5) finally 2 extra works done related to keeping the settings panel visible when opening the modal as seems like it's a better UX to keep this option visible rather than keeping them hidden and then requiring 1 extra click to make them open. To achieve this set the initial default value to 'true' for 'settingsOpen' useState & removed the 'setSettingsOpen(false)' from 'stopEditing' function.

*** Thanks 😊😊 ***
@hz-tyfoon hz-tyfoon requested a review from getdave as a code owner May 12, 2023 16:17
@skorasaurus skorasaurus added the [Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) label May 12, 2023
@hz-tyfoon
Copy link
Contributor Author

Here's a video demo of the Fixs:

link_control_issue_fix.mp4

cc: @getdave @annezazu @supernovia @jordesign
Feel free to share your thoughts/reviews/feedbacks. Thanks 😊 🙏

title: internalTextInputValue,
};
setNewValue( valueToSubmit );
onChange( valueToSubmit );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original code didn't trigger onChange if nothing changed. Is this still true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review 😊 🙏 @draganescu
When submitted, Here I triggered onChange even the the 'title' & 'url' wasn't changed. I described the following in my PR's description that's I believe related to your question:

** 3) in 'handleSubmit' of 'index.js' removed the if check as looks like it's unnecessary. Because if user decide to submit without making any change to 'url' or 'title' there's no harm in still submitting it, also there's other change can happen like 'open in new tab settings' and user may just wanna submit that.

Copy link
Contributor Author

@hz-tyfoon hz-tyfoon May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an update: I haven't found any issue caused by this so far. I'm assuming U haven't too.
But if U see any issue or potential issue/problem with this changes please feel free to share.

I'm not resolving this conversation at this moment. But looks so far it can be resolved if no issue found from your end.
Thanks again @draganescu . 😊

@draganescu
Copy link
Contributor

I tested this and stumbled upon this apparently wrong behavior:

cancel-link-ui-wrong.mp4

Clicking "Cancel" should close LinkUI with no changed saved in no state.

@hz-tyfoon
Copy link
Contributor Author

thanks again @draganescu for catching it 🙏 . Think I found where it's coming from. Just, little occupied now but, will push a fix soon I get a chance.

 - fixed: when making some change after opening the 'link control modal' & without applying/submitting it, when the 'cancel' button is clicked it show's that the changes already applied even though it's cancelled (although the updated changes aren't really applied but, still it's a misleading UI so fixed it)
@hz-tyfoon
Copy link
Contributor Author

I tested this and stumbled upon this apparently wrong behavior:

Clicking "Cancel" should close LinkUI with no changed saved in no state.

@draganescu
Fixed the issue👆you shared in this commit: 2b35bad . Can U check & confirm please? 🙏
Thanks a lot for testing the PR & finding it 😊

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for continuing to iterate on this.

I think we need to reduce the number of changes in this PR to keep it focused on doing a single thing. It's great to have your help in addressing so many issues but it makes the PR harder to review and also makes it harder to identify and rollback PRs that cause regressions. In general we advocate for smaller PRs that do one thing. Let's remove 4 and 5 as a minimum - feel free to spin out other PRs if you are able.

I've left some initial comments. Don't feel you have to address those immediately. I'm going to dive in more today and look at this in more detail. Appreciate your patience and continued work here 🙇

…e it's better to handle it separately from 'PR 50602'

*** What was the 'fix/item: 4' that's reverted in this commit? here's what I wrote about it in my description 👇👇 ***

"imported the 'Settings' component inside 'index.js' and used it below the 'LinkPreview' component. used the neccessary css in 'style.scss' to properly style it. In this 'Settings' component's 'onChange' prop, passed a function & made sure the value gets called with both 'setNewValue' and 'onChnge'. This is done for the same reason of keeping the both state sync. This settings component showen there to keep the 'open in new tab option' visible when previewing the link (after initially any richtext is linked by selecting the text & then pasting the link ). This was done to keep it consistent with 'WordPress 6.2 core UI' and looks like many people find it better UX to keep the 'option' there. See these comments for more info: this comment, also this comment with a video demonstration & this comment"
…e it's better to handle it separately from 'PR 50602'

*** What was the 'fix/item: 5' that's removed in this commit? here's what I wrote about it in my description 👇👇 ***

"finally 2 extra works done related to keeping the settings panel visible when opening the modal as seems like it's a better UX to keep this option visible rather than keeping them hidden and then requiring 1 extra click to make them open. To achieve this set the initial default value to 'true' for 'settingsOpen' useState & removed the 'setSettingsOpen(false)' from 'stopEditing' function."
… 'setNewValue' but passed the 'setNewValue function' as suggested by dave

I inititally renamed both the prop 'name and value' to 'setNewValue' so that it prevents any confusion being created (like, is the recieved 'onChange' prop in 'settings-drawe.js' is the same one which was recieved in the 'index.js'?) But, looks like that's not something to worry about at this moment. Looks like it's better to have that disscussion separately from this PR
@hz-tyfoon
Copy link
Contributor Author

Thanks again @getdave for the explanation & the review. 😊
Really sorry 😞 about this 1 PR addressing multiple things. After your explanation looks like it's better to keep concerns separate from this PR.

I think we need to reduce the number of changes in this PR to keep it focused on doing a single thing. It's great to have your help in addressing so many issues but it makes the PR harder to review and also makes it harder to identify and rollback PRs that cause regressions. In general we advocate for smaller PRs that do one thing. Let's remove 4 and 5 as a minimum - feel free to spin out other PRs if you are able.

I removed 4 & 5 in c5eb7dc & e3ce2c6 respectively.

I'll create separate issue/PR for both of them later.

Thanks. 😊
Checking your other reviews as well.

… suggested links closes the modal) as per review

Why?

To keep concerns separate. Think It'd be better if this is disscussed separately & this PR shouldn't look to change this behaviour. more by dave here: https://github.com/WordPress/gutenberg/pull/50602\#discussion_r1194818569
@hz-tyfoon
Copy link
Contributor Author

I addressed all of your reviews so far, can U check 🙏 Thanks.

@getdave
Copy link
Contributor

getdave commented May 17, 2023

Thank you. On my list to review...

@getdave
Copy link
Contributor

getdave commented May 24, 2023

Let's take a moment to consider the proposed UX/UI refresh of Link Control in #50891 before we decide how to proceed here.

@richtabor
Copy link
Member

@getdave is this still relevant, given the current state of link control?

@getdave
Copy link
Contributor

getdave commented Oct 17, 2023

I believe all the Issues described here are resolved by our work in upstream repo. Safe close out.

@getdave getdave closed this Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants